Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JDK21] Add support for JVMTI PopFrame #17809

Merged
merged 1 commit into from
Jul 25, 2023
Merged

[JDK21] Add support for JVMTI PopFrame #17809

merged 1 commit into from
Jul 25, 2023

Conversation

babsingh
Copy link
Contributor

Previously, PopFrame returned JVMTI_ERROR_OPAQUE_FRAME for a virtual
thread.

In JDK21, PopFrame includes support for virtual threads as per the
JVMTI specification:

  • Error if a virtual thread is not suspended and not the current
    thread.
  • Error if a virtual thread is unomunted since it won't be able to
    pop the current frame.
  • For a carrier thread with a virtual thread mounted, the details of
    the carrier thread are derived from targetThread->currentContinuation.

Related:

Also, there is no need to halt and resume a thread for inspection since
PopFrame expects the thread to be suspended as per the JVMTI spec. If a
thread is not suspended, it returns JVMTI_ERROR_THREAD_NOT_SUSPENDED.

@babsingh
[JDK21] Add support for JVMTI PopFrame
b318f52 
Previously, PopFrame returned JVMTI_ERROR_OPAQUE_FRAME for a virtual
thread.

In JDK21, PopFrame includes support for virtual threads as per the
JVMTI specification:
- Error if a virtual thread is not suspended and not the current
  thread.
- Error if a virtual thread is unomunted since it won't be able to
  pop the current frame.
- For a carrier thread with a virtual thread mounted, the details of
  the carrier thread are derived from targetThread->currentContinuation.

Related:
- eclipse-openj9#17715
- eclipse-openj9#17716

Also, there is no need to halt and resume a thread for inspection since
PopFrame expects the thread to be suspended as per the JVMTI spec. If a
thread is not suspended, it returns JVMTI_ERROR_THREAD_NOT_SUSPENDED.

Signed-off-by: Babneet Singh <[email protected]>
@babsingh babsingh marked this pull request as draft July 17, 2023 19:26
@babsingh babsingh marked this pull request as ready for review July 18, 2023 14:05
@babsingh
Copy link
Contributor Author

Also, there is no need to halt and resume a thread for inspection since PopFrame expects the thread to be suspended as per the JVMTI spec. If a thread is not suspended, it returns JVMTI_ERROR_THREAD_NOT_SUSPENDED.

The above requirement to suspend is enforced from JDK8 to JDK21:

JDK8 - https://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html#PopFrame
JDK21 - https://download.java.net/java/early_access/jdk21/docs/specs/jvmti.html#PopFrame

@babsingh
Copy link
Contributor Author

Verified locally that PopFrameTest passes with the changes from this PR.

@tajila Requesting your review.

@babsingh babsingh requested a review from tajila July 19, 2023 17:45
tajila
tajila reviewed Jul 20, 2023
View reviewed changes
runtime/jvmti/jvmtiStackFrame.c
currentThread, thread, &targetThread, JVMTI_ERROR_OPAQUE_FRAME,
J9JVMTI_GETVMTHREAD_ERROR_ON_NULL_JTHREAD | J9JVMTI_GETVMTHREAD_ERROR_ON_DEAD_THREAD | J9JVMTI_GETVMTHREAD_ERROR_ON_VIRTUALTHREAD);
currentThread, thread, &targetThread, JVMTI_ERROR_NONE,
J9JVMTI_GETVMTHREAD_ERROR_ON_NULL_JTHREAD | J9JVMTI_GETVMTHREAD_ERROR_ON_DEAD_THREAD);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it legal to continue if its a dead thread?

Copy link
Contributor Author

@babsingh babsingh Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, JVMTI_ERROR_THREAD_NOT_ALIVE will be returned by getVMThread when the J9JVMTI_GETVMTHREAD_ERROR_ON_DEAD_THREAD flag is set.

runtime/jvmti/jvmtiStackFrame.c
/* Error if the thread is not suspended and not the current thread. */
if ((currentThread != targetThread)
#if JAVA_SPEC_VERSION >= 21
&& (0 == J9OBJECT_U32_LOAD(currentThread, threadObject, vm->isSuspendedInternalOffset))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we be checking current thread or target thread?

Copy link
Contributor Author

@babsingh babsingh Jul 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check if the input/target thread is suspended. If thread != currentThread && thread is not suspended, then JVMTI_ERROR_THREAD_NOT_SUSPENDED is returned.

@tajila
Copy link
Contributor

tajila commented Jul 24, 2023

jenkins compile win jdk11

@tajila
Copy link
Contributor

tajila commented Jul 24, 2023

jenkins test sanity alinux64 jdk21

@tajila tajila merged commit 15f5859 into eclipse-openj9:master Jul 25, 2023
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Jul 25, 2023
@babsingh
[JDK21] Enable JVMTI tests
17e0a93 
- StopThreadTest fixed by eclipse-openj9/openj9#17806
- PopFrameTest fixed by eclipse-openj9/openj9#17809
- GetSetLocalUnsuspended fixed by eclipse-openj9/openj9#17829

Closes eclipse-openj9/openj9#17711
Closes eclipse-openj9/openj9#17715
Closes eclipse-openj9/openj9#17716
Closes eclipse-openj9/openj9#17717
Closes eclipse-openj9/openj9#17718

Signed-off-by: Babneet Singh <[email protected]>
@babsingh babsingh mentioned this pull request Jul 25, 2023
Merged
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Jul 25, 2023
@babsingh
[JDK21] Enable JVMTI tests
112649c 
- StopThreadTest is fixed by eclipse-openj9/openj9#17806.
- PopFrameTest is fixed by eclipse-openj9/openj9#17809.
- GetSetLocalUnsuspended is fixed by eclipse-openj9/openj9#17829.

Closes eclipse-openj9/openj9#17711
Closes eclipse-openj9/openj9#17715
Closes eclipse-openj9/openj9#17716
Closes eclipse-openj9/openj9#17717
Closes eclipse-openj9/openj9#17718

Signed-off-by: Babneet Singh <[email protected]>
llxia pushed a commit to adoptium/aqa-tests that referenced this pull request Jul 25, 2023
@babsingh
[JDK21] Enable JVMTI tests (#4684)
ffa0db5 
- StopThreadTest is fixed by eclipse-openj9/openj9#17806.
- PopFrameTest is fixed by eclipse-openj9/openj9#17809.
- GetSetLocalUnsuspended is fixed by eclipse-openj9/openj9#17829.

Closes eclipse-openj9/openj9#17711
Closes eclipse-openj9/openj9#17715
Closes eclipse-openj9/openj9#17716
Closes eclipse-openj9/openj9#17717
Closes eclipse-openj9/openj9#17718

Signed-off-by: Babneet Singh <[email protected]>
@babsingh babsingh mentioned this pull request Jul 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tajila tajila tajila approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@babsingh @tajila